Skip to content

[CLEAN] Synthetic Benchmark PR #102472 - feat: Consolidate adjacent traces & move next to search #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
tomerqodo wants to merge 1 commit into
base: base_pr_102472_20251204_9590
Choose a base branch
from
Open

[CLEAN] Synthetic Benchmark PR #102472 - feat: Consolidate adjacent traces & move next to search #13

tomerqodo wants to merge 1 commit into from

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025 •
edited by qodo-code-review bot
Loading

User description

Benchmark PR getsentry#102472

Type: Clean (correct implementation)

Original PR Title: feat: Consolidate adjacent traces & move next to search
Original PR Description: ## Description

This is a follow up to getsentry#96510. The new approach is that the buttons are close to the other interaction buttons, so the user doesn't have to go and find this at the very bottom of the page. Since the search and the "Open in Explore" are already trace related, the user doesn't have to switch "context" and the buttons wouldn't need any additional text in it, except the tooltip if users are curious what it is.

Significant changes

  • useFindPreviousTrace and useFindNextTrace have been consolidated into useFindAdjacentTrace, which reduces code duplication
  • The tooltip has been moved to the outer most ButtonBar, since it wouldn't need a tooltip for each button
  • A own component has been created for the Buttons so it can be easily moved around

📹

The UI reacts a little slow in the GIF since my computer was a little low on resources at this point in time

Kapture 2025-10-31 at 13 22 55

Original PR URL: getsentry#102472

PR Type

Enhancement

Description

  • Consolidate useFindNextTrace and useFindPreviousTrace into unified useFindAdjacentTrace hook

  • Move trace navigation buttons to toolbar with shared tooltip and ButtonBar component

  • Simplify button rendering using LinkButton component with icon and aria-label

  • Extract trace links navigation logic into dedicated TraceLinksNavigation component

Diagram Walkthrough

flowchart LR
  A["useFindNextTrace<br/>useFindPreviousTrace"] -->|consolidate| B["useFindAdjacentTrace"]
  C["TraceLinkNavigationButton<br/>with Tooltips"] -->|simplify| D["LinkButton<br/>in ButtonBar"]
  E["Bottom of page<br/>trace links"] -->|move| F["Toolbar<br/>with Search & Explore"]
  B --> D
  D --> F
Loading

File Walkthrough
Relevant files
Enhancement
useFindLinkedTraces.ts
Consolidate adjacent trace finding logic                                 

static/app/views/performance/newTraceDetails/traceLinksNavigation/useFindLinkedTraces.ts

  • Consolidate useFindNextTrace and useFindPreviousTrace into single
    useFindAdjacentTrace function
  • Unify parameter naming with adjacentTrace* prefix for both directions
  • Implement direction-based logic for search queries and enabled
    conditions
  • Return consistent object structure with available, isLoading, id, and
    trace fields
 +81/-120
traceLinkNavigationButton.tsx
Simplify button rendering with LinkButton component           

static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx

  • Replace individual tooltip and styled link components with LinkButton
    from scraps
  • Simplify button rendering to single component without conditional
    logic
  • Remove placeholder component and styling code
  • Use unified useFindAdjacentTrace hook instead of two separate hooks
  • Add iconDirection and ariaLabel computed values based on direction
 +48/-133
traceLinksNavigation.tsx
Extract trace links navigation into dedicated component   

static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinksNavigation.tsx

  • Create new component to wrap trace navigation buttons in ButtonBar
    with shared tooltip
  • Consolidate feature flag and source checks into single component
  • Move tooltip with documentation link to container level
  • Render both previous and next trace buttons together with merged
    button styling
 +65/-0   
traceWaterfall.tsx
Move trace navigation buttons to toolbar                                 

static/app/views/performance/newTraceDetails/traceWaterfall.tsx

  • Remove showLinkedTraces state logic and move to TraceLinksNavigation
    component
  • Replace bottom-of-page trace links container with toolbar-integrated
    TraceLinksNavigation
  • Remove TraceLinkNavigationButton and
    TraceLinkNavigationButtonPlaceHolder imports
  • Remove TraceLinksNavigationContainer styled component
  • Simplify layout by eliminating fragment and conditional rendering at
    bottom
 +5/-47   

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The PR adds UI navigation actions and span queries without adding any audit logging, which
may be acceptable for non-critical UI interactions but cannot be confirmed from this diff.

Referred Code 
return (
  <Tooltip
    position="top"
    delay={400}
    isHoverable
    title={tct(
      `Go to the previous or next trace of the same session. [link:Learn More]`,
      {
        link: (
          <ExternalLink href="https://docs.sentry.io/concepts/key-terms/tracing/trace-view/#previous-and-next-traces" />
        ),
      }
    )}
  >
    <ButtonBar merged gap="0">
      <TraceLinkNavigationButton
        direction="previous"
        attributes={rootEventResults.data.attributes}
        currentTraceStartTimestamp={
          new Date(rootEventResults.data.timestamp).getTime() / 1000
        }


 ... (clipped 12 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Limited error handling: The hook relies on isError from useSpans but surfaces only availability/loading to
consumers without conveying error context, which may be acceptable for UI but reduces
debuggability.

Referred Code 
const spanId = data?.[0]?.id;
const traceId = data?.[0]?.trace;

return useMemo(() => {
  if (direction === 'next') {
    return {
      id: spanId,
      trace: traceId,
      available: !!spanId && !!traceId && !isError,
      isLoading: isPending,
    };
  }

  return {
    trace: adjacentTraceId,
    id: adjacentTraceSpanId,
    available: !!data?.[0]?.id && !isError,
    isLoading: isPending,
  };
}, [
  direction,


 ... (clipped 9 lines)

Learn more about managing compliance generic rules or creating your own custom rules
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid generating an invalid URL

Conditionally set the to prop on LinkButton to undefined when traceId is missing
to avoid generating an invalid URL for a disabled button.

static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx [88-102]

 <LinkButton
   size="xs"
   icon={<IconChevron direction={iconDirection} />}
   aria-label={ariaLabel}
   onClick={closeSpanDetailsDrawer}
   disabled={!traceId || isTraceLoading || !isTraceAvailable}
-  to={getTraceDetailsUrl({
-    traceSlug: traceId ?? '',
-    spanId: traceSpanId,
-    dateSelection,
-    timestamp: linkedTraceWindowTimestamp,
-    location,
-    organization,
-  })}
+  to={
+    traceId
+      ? getTraceDetailsUrl({
+          traceSlug: traceId,
+          spanId: traceSpanId,
+          dateSelection,
+          timestamp: linkedTraceWindowTimestamp,
+          location,
+          organization,
+        })
+      : undefined
+  }
 />
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out that a potentially invalid URL is generated for the to prop when the button is disabled. The proposed fix prevents this, improving code robustness.

Low
General
Avoid redundant timestamp calculation

Calculate the currentTraceStartTimestamp value once and store it in a variable
to avoid redundant computation for the two TraceLinkNavigationButton components.

static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinksNavigation.tsx [48-61]

-<TraceLinkNavigationButton
-  direction="previous"
-  attributes={rootEventResults.data.attributes}
-  currentTraceStartTimestamp={
-    new Date(rootEventResults.data.timestamp).getTime() / 1000
-  }
-/>
-<TraceLinkNavigationButton
-  direction="next"
-  attributes={rootEventResults.data.attributes}
-  currentTraceStartTimestamp={
-    new Date(rootEventResults.data.timestamp).getTime() / 1000
-  }
-/>
+const currentTraceStartTimestamp =
+  new Date(rootEventResults.data.timestamp).getTime() / 1000;
 
+return (
+  <Tooltip
+    position="top"
+    delay={400}
+    isHoverable
+    title={tct(
+      `Go to the previous or next trace of the same session. [link:Learn More]`,
+      {
+        link: (
+          <ExternalLink href="https://docs.sentry.io/concepts/key-terms/tracing/trace-view/#previous-and-next-traces" />
+        ),
+      }
+    )}
+  >
+    <ButtonBar merged gap="0">
+      <TraceLinkNavigationButton
+        direction="previous"
+        attributes={rootEventResults.data.attributes}
+        currentTraceStartTimestamp={currentTraceStartTimestamp}
+      />
+      <TraceLinkNavigationButton
+        direction="next"
+        attributes={rootEventResults.data.attributes}
+        currentTraceStartTimestamp={currentTraceStartTimestamp}
+      />
+    </ButtonBar>
+  </Tooltip>
+);
+
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a duplicated calculation and proposes a valid refactor that improves code readability and maintainability by removing the redundancy.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tomerqodo